Conversation
f5ee523 to
8ff5627
Compare
e-koch
left a comment
There was a problem hiding this comment.
Added a few comments but have concerns over required/optional dependencies (that we've arleady started discussing offline).
6cd7782 to
ba94987
Compare
9761ffb to
5202830
Compare
|
Note to self after ADR today: Once comments have been implemented, run some test imaging end-to-end on monolithic/pip-installed CASA versions on Mac/Linux machine |
e-koch
left a comment
There was a problem hiding this comment.
Thanks @thomaswilliamsastro ! I'm happy with moving forward with merging and pushing development in v4.X.
ecae707 to
76a1ef8
Compare
|
End-to-end testing:
So I think we're basically ready to merge here. Note that I've had to pin one dependency, |
76a1ef8 to
4c0f24e
Compare
|
@e-koch I've just done the mammoth rebase and conflict resolution. I'm running through end-to-end on one test case to make sure nothing weird has been introduced, but it would be good to get eyes over this again quickly |
59b2675 to
3d9a2b2
Compare
|
@thomaswilliamsastro -- we should update the README here, too. And a clear link to the v3.2 release for prior CASA version. |
3d9a2b2 to
912c481
Compare
@e-koch Done. Also edited the links to point at the new repo wherever they appeared |
217a1d7 to
8ba32cf
Compare
e-koch
left a comment
There was a problem hiding this comment.
@thomaswilliamsastro this is looking great! I added a couple minor comments.
The only part I have a lingering concern about is triggering a spectral-cube import at the package level for the derived handler. I'd rather we keep an if/else check there for whether spectral-cube is installed as it'll become the main friction point for most people still using the pipeline in a monolithic environment.
| import numpy as np | ||
|
|
||
| from . import handlerTemplate | ||
| from . import utilsFilenames | ||
| from .scConvolution import smooth_cube |
There was a problem hiding this comment.
These still need a check for whether spectral-cube is installed so we should migrate this back into the init or have a file-wide if/else. Otherwise the package level imports will fail in a monolithic CASA environment.
There was a problem hiding this comment.
Sorted, I think -- I've added a function to check spectral_cube is importable, and only then will the derivedHandler import
| pip install -e . | ||
| ``` | ||
|
|
||
| If you are using a monolithic CASA installation, you can run this within |
There was a problem hiding this comment.
We should give a few more details here. Probably linking to https://casadocs.readthedocs.io/en/stable/notebooks/frequently-asked-questions.html#Astropy-in-monolithic-CASA
and noting that the old "append to path" in the config.py file is still an option (since that's how most people will likely continue to use for a bit to not break existing workflows)
There was a problem hiding this comment.
Added a link here
- Fix step occasionally being 0 in noise cube generation - Add dependabot, and build actions to ensure package installs correctly on Python matrix - Add CODEOWNERS, so PRs get assigned automatically - Update CITATION.md to CITATION.cff, and format correctly - Rename LICENSE.rst to just LICENSE - Rewrite pyproject.toml to allow pip-installation - Expand list of authors/maintainers/citation.cff - Rename README.rst to README.md - Significant reformatting and updating to reflect packaging - Include details on chunked imaging - Remove setup.cfg/setup.py - Remove unused _astropy_init.py - Remove unused conftest.py - Remove unused data/ directory in phangsPipeline - Remove unused and very outdated PHANGSPipelineReadme.md in phangsPipeline - Moved casaBlankCleanRecipe to scripts, and renamed - Refactored versioning throughout, now pulls automatically from the package - Fully align CASA version throughout - General import tidy-up - Move various example scripts into scripts/ directory - Consolidate run script into run_pipeline_phangs-alma.py - Update README.md to include link to v3.2 - Move links from akleroy->PhangsTeam throughout - Add CHANGES.rst, and workflow to make sure CHANGES.rst has been edited on a PR - Address comments from @e-koch
8ba32cf to
2fff257
Compare
|
@e-koch Comments addressed, I haven't resolved the two potentially more sticky points but they should be fine now (I hope) |
Pretty sizeable refactor, but this enables pip-installing the pipeline, which means we can finally keep a handle on python/CASA versions.
I've transferred the author list from the paper, and maintainers from the contributors list. If there's anyone missing, please let me know!